Spec optional conversion to RGB in VideoFrame.copyTo() #754

Djuffin · 2023-12-07T07:18:30Z

This change should address the issue of pixel format conversion in VideoFrame.copyTo().

Djuffin · 2023-12-07T07:22:15Z

There are probably some missing details, typos and omissions.
But I'd love to get some feedback on the general approach in this change.

index.src.html

Djuffin · 2024-02-29T05:23:56Z

@padenot Could you please take a look? I'd love to merge this PR some time soon. We've talked about it at TPAC last September.

padenot · 2024-02-29T09:24:45Z

index.src.html

@@ -4179,10 +4205,36 @@
            [=combined buffer layout/allocationSize=].
    9. Return |combinedLayout|.

+: <dfn for=VideoFrame>Convert to RGB frame</dfn> (with |format|, |rect| and |colorSpace|)
+ :: 1. Let |canvas| be a new {{OffscreenCanvas}} constructed with
+        |rect|.|width| and |rect|.|height|.


This isn't something that we can do in spec, using other specs directly like that. All the objects you use can have been overridden in the global this algorithm is running in.

We should use internal stops for canvas instead.

I'm probably missing something here. Could you elaborate a bit?

Isn't it what we do in the Rendering section?

referring to CanvasDrawImage drawImage()

No, that's unrelated, we reference it in your link. What happens when drawImage is called with a VideoFrame is well-defined there. Here you're using another Web Platform object from the innards of our spec.

e.g. what happens if in the document we're doing this conversion, I've maliciously done:

window.OffscreenCanvas.prototype = function() { alert("what now"); };

before anything from our spec run?

This algorithm does not refer to any particular js code. It explicitly says what object needs to be created by the user agent. That's why I don't think that global overrides should concern us here. With a similar approach half of this spec can be put into question with code like

window.VideoFrame.prototype = function() { alert("what now"); };

I added a note clarifying that UAs don't have to do it via literally creating an OffscreenCanvas as long as the result is the same. I hope this eliminates possible confusion.

If you still think that the wording leaves room for misinterpretation, I'm very much open to suggestions how to rewrite it.

Thanks for the links!

I've noticed that drawImage() (step 6) does not specify any details about the rendering (sampling, color conversion etc). It's only saying that "The user agent may use any filtering algorithm".
So there isn't really an algorithm to extract and call.
I will have to rewrite it in a more abstract manner, similar to the text for drawImage()

We can add an export to something hand-wavy from canvas now, and merge what we want to do here, and then fix the canvas spec as well, your call.

I asked HTML spec owners about the best way to reuse canvas pixel format conversion algorithm.

I've looked at the rest of the WebCodecs spec. It's pretty light on the details when it comes to format and color spaces conversions. (for example Rendering and AudioData.copyTo )

So I rewrote "Convert to RGB frame" in the similar manner.
Please take another look.

@padenot gentle ping

index.src.html

chrisn

Looks good, but let's resolve the discussion on overridden globals (I don't mind if that's here or in a follow up PR).

Co-authored-by: Chris Needham <[email protected]>

index.src.html

Djuffin · 2024-04-18T09:48:02Z

@padenot PTAL

padenot

All good, thanks!

Djuffin · 2024-04-18T22:44:36Z

@aboba please take a look and merge if you don't have new comments.

SHA: 48523cd Reason: push, by aboba Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

…ayback-reviewers,smaug,padenot This patch updates the `VideoFrameCopyToOptions` webidl to meet the changes in WebCodecs PR 754 [1]. [1] w3c/webcodecs#754 Differential Revision: https://phabricator.services.mozilla.com/D213805

…viewers,padenot This patch updates the `ParseVideoFrameCopyToOptions` to the changes made in WebCodecs PR 754 [1]. The changes make the caller of `ParseVideoFrameCopyToOptions` throw a `NotSupported` error when the given `VideoFrameCopyToOptions`'s format is invalid. [1] w3c/webcodecs#754 Differential Revision: https://phabricator.services.mozilla.com/D213806

…ayback-reviewers,smaug,padenot This patch updates the `VideoFrameCopyToOptions` webidl to meet the changes in WebCodecs PR 754 [1]. [1] w3c/webcodecs#754 Differential Revision: https://phabricator.services.mozilla.com/D213805

…viewers,padenot This patch updates the `ParseVideoFrameCopyToOptions` to the changes made in WebCodecs PR 754 [1]. The changes make the caller of `ParseVideoFrameCopyToOptions` throw a `NotSupported` error when the given `VideoFrameCopyToOptions`'s format is invalid. [1] w3c/webcodecs#754 Differential Revision: https://phabricator.services.mozilla.com/D213806

…ayback-reviewers,smaug,padenot This patch updates the `VideoFrameCopyToOptions` webidl to meet the changes in WebCodecs PR 754 [1]. [1] w3c/webcodecs#754 Differential Revision: https://phabricator.services.mozilla.com/D213805

…viewers,padenot This patch updates the `ParseVideoFrameCopyToOptions` to the changes made in WebCodecs PR 754 [1]. The changes make the caller of `ParseVideoFrameCopyToOptions` throw a `NotSupported` error when the given `VideoFrameCopyToOptions`'s format is invalid. [1] w3c/webcodecs#754 Differential Revision: https://phabricator.services.mozilla.com/D213806

Djuffin requested review from aboba and padenot December 7, 2023 07:20

Djuffin self-assigned this Dec 7, 2023

sandersdan reviewed Dec 7, 2023

View reviewed changes

index.src.html Outdated Show resolved Hide resolved

sandersdan reviewed Dec 7, 2023

View reviewed changes

index.src.html Outdated Show resolved Hide resolved

sandersdan reviewed Dec 7, 2023

View reviewed changes

index.src.html Outdated Show resolved Hide resolved

sandersdan reviewed Dec 7, 2023

View reviewed changes

index.src.html Outdated Show resolved Hide resolved

Djuffin commented Dec 8, 2023

View reviewed changes

index.src.html Outdated Show resolved Hide resolved

chrisn requested changes Dec 12, 2023

View reviewed changes

sandersdan reviewed Dec 14, 2023

View reviewed changes

index.src.html Show resolved Hide resolved

Djuffin force-pushed the rgb branch from 4f99d19 to b8ca480 Compare February 29, 2024 05:18

padenot requested changes Feb 29, 2024

View reviewed changes

Djuffin requested a review from chrisn February 29, 2024 16:59

chrisn approved these changes Mar 5, 2024

View reviewed changes

Djuffin mentioned this pull request Mar 19, 2024

Possibility to export drawImage() rendering algorithm whatwg/html#10210

Open

Djuffin and others added 9 commits April 8, 2024 16:09

Spec optional conversion to RGB in VideoFrame.copyTo()

cc00c96

More clear writing

ce2d6ae

cab -> can

8e5f883

More clear handling of |rect| in RGB branch of VideoFrame.copyTo

36a7447

More specific language around VideoFrameCopyToOptions.colorSpace

ee8cd68

Editorial changes from Chris Needham

51dbdad

Co-authored-by: Chris Needham <[email protected]>

Allow for color space conversion to p3 and sRGB for RGB frames as well

875bc88

Addressing review comments

83a92e4

Rewritten "Convert to RGB frame" algo

33f79ae

Djuffin force-pushed the rgb branch from 1220f21 to 33f79ae Compare April 10, 2024 01:05

Kaiido reviewed Apr 10, 2024

View reviewed changes

index.src.html Outdated Show resolved Hide resolved

Reject promise instead of throwing

142c506

padenot reviewed Apr 15, 2024

View reviewed changes

drop extra the

0df4aee

Djuffin requested a review from padenot April 16, 2024 09:16

padenot approved these changes Apr 18, 2024

View reviewed changes

aboba merged commit 48523cd into w3c:main Apr 19, 2024
17 checks passed

github-actions bot added a commit that referenced this pull request Apr 19, 2024

Merge pull request #754 from Djuffin/rgb

d78e9d1

SHA: 48523cd Reason: push, by aboba Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot added a commit that referenced this pull request Apr 19, 2024

Merge pull request #754 from Djuffin/rgb

48adbf5

SHA: 48523cd Reason: push, by aboba Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot added a commit that referenced this pull request Apr 19, 2024

Merge pull request #754 from Djuffin/rgb

e8d4dcd

SHA: 48523cd Reason: push, by aboba Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot added a commit that referenced this pull request Apr 19, 2024

Merge pull request #754 from Djuffin/rgb

4e948d6

SHA: 48523cd Reason: push, by aboba Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot added a commit that referenced this pull request Apr 19, 2024

Merge pull request #754 from Djuffin/rgb

1d326c4

SHA: 48523cd Reason: push, by aboba Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot added a commit that referenced this pull request Apr 19, 2024

Merge pull request #754 from Djuffin/rgb

6a6034e

SHA: 48523cd Reason: push, by aboba Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot added a commit that referenced this pull request Apr 19, 2024

Merge pull request #754 from Djuffin/rgb

3011d1b

SHA: 48523cd Reason: push, by aboba Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot added a commit that referenced this pull request Apr 19, 2024

Merge pull request #754 from Djuffin/rgb

94e40a5

SHA: 48523cd Reason: push, by aboba Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot added a commit that referenced this pull request Apr 19, 2024

Merge pull request #754 from Djuffin/rgb

8d4efce

SHA: 48523cd Reason: push, by aboba Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot added a commit that referenced this pull request Apr 19, 2024

Merge pull request #754 from Djuffin/rgb

759edea

SHA: 48523cd Reason: push, by aboba Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot added a commit that referenced this pull request Apr 19, 2024

Merge pull request #754 from Djuffin/rgb

8cca561

SHA: 48523cd Reason: push, by aboba Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot added a commit that referenced this pull request Apr 19, 2024

Merge pull request #754 from Djuffin/rgb

d41ca5c

SHA: 48523cd Reason: push, by aboba Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot added a commit that referenced this pull request Apr 19, 2024

Merge pull request #754 from Djuffin/rgb

c094953

SHA: 48523cd Reason: push, by aboba Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot added a commit that referenced this pull request Apr 19, 2024

Merge pull request #754 from Djuffin/rgb

186a093

SHA: 48523cd Reason: push, by aboba Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot added a commit that referenced this pull request Apr 19, 2024

Merge pull request #754 from Djuffin/rgb

41fe494

SHA: 48523cd Reason: push, by aboba Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot added a commit that referenced this pull request Apr 19, 2024

Merge pull request #754 from Djuffin/rgb

8d3aec4

SHA: 48523cd Reason: push, by aboba Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

jyasskin mentioned this pull request Sep 4, 2024

Type mismatch in colorSpace parameter to Convert to RGB frame #831

Closed

Djuffin deleted the rgb branch September 10, 2024 23:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spec optional conversion to RGB in VideoFrame.copyTo() #754

Spec optional conversion to RGB in VideoFrame.copyTo() #754

Djuffin commented Dec 7, 2023 •

edited by pr-preview bot

Loading

Djuffin commented Dec 7, 2023

Djuffin commented Feb 29, 2024 •

edited

Loading

padenot Feb 29, 2024

Djuffin Feb 29, 2024

padenot Feb 29, 2024

Djuffin Feb 29, 2024

Djuffin Feb 29, 2024

Djuffin Mar 5, 2024

padenot Mar 5, 2024

Djuffin Mar 21, 2024

Djuffin Apr 10, 2024

Djuffin Apr 11, 2024

chrisn left a comment

Djuffin commented Apr 18, 2024

padenot left a comment

Djuffin commented Apr 18, 2024 •

edited

Loading

Spec optional conversion to RGB in VideoFrame.copyTo() #754

Spec optional conversion to RGB in VideoFrame.copyTo() #754

Conversation

Djuffin commented Dec 7, 2023 • edited by pr-preview bot Loading

Djuffin commented Dec 7, 2023

Djuffin commented Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisn left a comment

Choose a reason for hiding this comment

Djuffin commented Apr 18, 2024

padenot left a comment

Choose a reason for hiding this comment

Djuffin commented Apr 18, 2024 • edited Loading

Djuffin commented Dec 7, 2023 •

edited by pr-preview bot

Loading

Djuffin commented Feb 29, 2024 •

edited

Loading

Djuffin commented Apr 18, 2024 •

edited

Loading